feat(http-transport-reqwest): allow tls options#7798
Conversation
dosubot
Bot
added
size:XL
|
@Xuanwo Can you have a look on the default feature proposed in PR? Then I can resolve build issues in CI. |
| # Use Rustls without a built-in crypto provider. | ||
| rustls-no-provider = ["reqwest/rustls-no-provider"] | ||
| # Use Rustls with the aws-lc-rs crypto provider and bundled Mozilla root certificates. | ||
| webpki-roots = [ |
There was a problem hiding this comment.
Should this be rustls-wekpki-roots?
| ```rust | ||
| use std::time::Duration; | ||
|
|
||
| use opendal_http_transport_reqwest::ReqwestTlsBackend; |
There was a problem hiding this comment.
I didn't expect we need to implement this. Is there a reason why we didn't just use things from reqwest?
There was a problem hiding this comment.
This is mainly a consideration of what features do we want to compile and ship. e.g.:
- we ship rustls, ring, all differents certs
- we ship one feature of rustls
- ...other combinations between these 2
Bindings will use this crate mostly so compilation size is also a concern.
There was a problem hiding this comment.
I realized that I'm writing a library instead of an app. I'll fix this soon.
Xuanwo
left a comment
Xuanwo
left a comment
There was a problem hiding this comment.
Thank you for working on this!
I love the direction, but I think we are taking on too many duties that do not belong to us. http-transport-reqwuest is just a simple wrapper. Users can just call reqwest's API to set it.
| } | ||
|
|
||
| #[allow(unreachable_code, clippy::needless_return)] | ||
| fn default_tls_backend() -> &'static str { |
There was a problem hiding this comment.
It's users duty to make sure they set those correctly.
There was a problem hiding this comment.
We should have a default builder helper to choose a TLS backend explicitly. It's useful for debugging.
There was a problem hiding this comment.
We should have a default builder helper to choose a TLS backend explicitly. It's useful for debugging.
I expect that users want to do this can always use reqwest's API.
I think about this in this way: if opendal supports 10 http clients in the future and every one of them has a default_tls_backend. It doesn't make sense to me.
I feel that it's best for use to not expose any new APIs from this crate. Users don't need to figure out opendal's API, they just need to play with reqwest as before.
Please note that opendal is highly possible that not the only crate in users system that depends on reqwest. Any new API, new wrapper from opendal will add more burden for users.
dosubot
Bot
added
size:L
| } | ||
|
|
||
| #[allow(unreachable_code, clippy::needless_return)] | ||
| fn default_tls_backend() -> &'static str { |
There was a problem hiding this comment.
We should have a default builder helper to choose a TLS backend explicitly. It's useful for debugging.
Xuanwo
left a comment
Xuanwo
left a comment
There was a problem hiding this comment.
Thank you @erickguan for working on this!
2 participants
Which issue does this PR close?
Closes #7762.
Rationale for this change
Allow easier configurations of TLS backend to reqwest.
What changes are included in this PR?
Are there any user-facing changes?
Yes, for http-transport-reqwest. The default configuration for http-transport-reqwest is to compile native-tls support.
Users who wants rustls must compile their own http-transport-reqwest.
AI Usage Statement
Claude Opus 4.6 helps implementation.